Skip to content

perf(lsp): parse-pipeline hardening — immutable docs, partial-result cache, cancellation, workspace pruning - #9

Merged
klaidliadon merged 9 commits into
masterfrom
ridl-lsp-hardening/parse-pipeline
Jun 20, 2026
Merged

klaidliadon merged 9 commits into
masterfrom
ridl-lsp-hardening/parse-pipeline

Conversation

@klaidliadon

@klaidliadon klaidliadon commented Jun 19, 2026 •

Copy link
Copy Markdown
Collaborator

The parse pipeline had a latent data race (the store handed out shared mutable *Document pointers), re-parsed the same buffer on every keystroke during edits, ignored cancellation entirely, and walked the whole workspace — .git/node_modules included — on every find-references. This hardens all four without changing behavior users rely on.

Second of the prod-readiness stack. Depends on #8 — merge that first.

⚠️ Stack: #8 → this PR. Do not merge before #8.


Changes (audit findings)

  • I1 — copy-on-write documents (store.go, server.go): DidChange builds a new *Document instead of mutating the shared one; Store.SetResult attaches parse results via COW with a version guard, so a slow parse of superseded content can't clobber a newer result. Latent race today (handlers are serialized by AsyncHandler), but the store no longer relies on that for safety.
  • I3 — retain partial results during edits (diagnostics.go): the parser produces a usable AST even on error; caching it lets navigation reuse the partial result instead of re-parsing the same invalid buffer per request.
  • I6 — context cancellation (parser.go, diagnostics.go): Parser.Parse takes a ctx and bails before each recursive import parse; the diagnostics path threads it through. A cancelled request no longer surfaces ctx.Err() as a diagnostic, clears diagnostics, or caches incomplete work.
  • I7 — bounded workspace scan (references.go): prune .git/node_modules/vendor/hidden dirs and surface walk errors via the logger instead of swallowing them.

Scope boundary (deliberate)

parsePathForNavigation (interactive nav) and interactive code-actions intentionally use context.Background() — single-file parses where threading ctx through the resolution-callback layer is high-churn for negligible benefit. The cancellable callers (diagnostics) use the ctx-aware parsePath.

Deferred (NOT in this PR — flagged for review)

  • I2 (debounce + parse-only-changed-doc) — risks cross-file diagnostic staleness (editing an imported file must refresh importers) and needs timer/coalescing concurrency. Wanted interactive-feel validation before landing, so it's held.
  • I7 follow-ups — threading cancellation into the WalkDir itself and a cached, watch-invalidated path index (needs file-watching plumbing).

Test plan

$ go test -race ./...
Go test: 130 passed in 6 packages

$ go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint run ./... -c .golangci.yml
0 issues.

New tests: COW invariant (document_cow_test.go), partial-result retention (repurposed diagnostics_test.go), SetResult version guard + missing-doc no-op (documents/store_test.go), parser + parsePath cancellation (parser_test.go, cancellation_test.go), workspace dir pruning (workspace_walk_test.go).

Known coverage gap: the WalkDir per-entry error-logging branch (references.go) is not unit-tested — injecting an unreadable entry hermetically is permission-dependent and flaky in CI. The pruning happy-path is covered; the error branch is by-inspection.

Review

Self-review caught a Critical (cancellation surfaced a bogus "context canceled" diagnostic) — fixed. A Codex adversarial pass (via agent-comms, 3 rounds to convergence) caught two more Important issues: cache poisoning when cancellation hit mid-import-recursion, and the diagnostics candidate-scan still escaping cancellation through the background parser. Both fixed; Codex approved the final revision. Security-review: no findings (WalkDir doesn't follow symlinks, so the scan is narrowed, not a new traversal surface).


Stack: ridl-lsp-hardening

  1. fix(lsp): crash-safety — panic recovery, layout canary, race+lint CI #8
  2. perf(lsp): parse-pipeline hardening — immutable docs, partial-result cache, cancellation, workspace pruning #9 ◀ this PR
  3. feat(lsp): polish — graceful shutdown, log level, upstream error-format canaries #10

This stack is managed with sdf.

The store guards its map with a mutex but handed out shared *Document pointers that DidChange and parseDocument then mutated in place (content, version, parse result). The mutex protects the map slots, not the structs they point to, so a reader holding an earlier snapshot could observe a torn update. It is latent today only because jsonrpc2.AsyncHandler serializes handler bodies (audit I1).

- DidChange now builds a new *Document instead of mutating the stored one, and clears the cached result since the content changed.

- Add Store.SetResult: attaches a parse result via copy-on-write with a version guard, so a stale parse is never cached and snapshots stay immutable. parseDocument uses it instead of mutating doc.Result, and passes the result explicitly to importDiagnostics.

- Test pins the invariant: a snapshot taken before DidChange keeps its content/version.
While a document is mid-edit and fails to parse, parseDocument cleared the cached result. Navigation then re-parsed the same invalid buffer on every request (parsePathForNavigation re-parses when the cache is empty) and produced the identical best-effort AST — so clearing bought nothing but repeated work (audit I3).

Cache the partial result (Root is populated even on parse errors) so definition/hover/completion reuse it instead of re-parsing per request. Diagnostics are unaffected — they still come from result.Errors. Repurposed the existing valid-to-invalid test to pin the retained-partial behavior.
Handlers ignored ctx entirely, so a client $/cancelRequest cancelled nothing and a long import-chasing parse always ran to completion (audit I6).

- Parser.Parse takes a context and checks it before each (recursive) parse, so a cancelled or superseded request stops walking the import graph promptly.

- The diagnostics path (parseDocument, importDiagnostics) threads the request ctx through.

- parsePathForNavigation deliberately keeps using context.Background(): it is a fast single-file parse, and threading ctx through the resolution-callback layer would be high-churn for negligible benefit. The workspace-walk cancellation (the other heavy path) is handled in the I7 commit.
find-references, workspace-symbols, and missing-import quick-fixes scan the workspace root synchronously on the request. The scan descended into every directory and swallowed walk errors (audit I7), so on a monorepo it walked .git/node_modules/vendor for nothing and a transient FS error silently truncated results while looking complete.

- Prune .git, node_modules, vendor, and hidden directories (they never hold project schemas).

- Log unreadable entries and an incomplete walk via the server logger instead of discarding the error.

Deferred (needs broader plumbing / file-watching, tracked with I2): threading request cancellation into the walk and a cached, watch-invalidated path index.
Every caller ignores the bool, so it was dead surface. Removing it also drops the lone what-comment from the godoc, leaving only the why.
After I6 threaded a cancellable ctx into the parser, a cancelled or superseded request made Parse return ctx.Err(), which parseDocument turned into a bogus line-1 "context canceled" diagnostic and cached a nil result — clobbering the document's real diagnostics mid-edit. Found in self-review.

Treat a context error as stop, not failure: parseDocument returns no diagnostics and leaves the cache intact, and parseAndPublishDiagnostics skips publishing when the request is already cancelled.
The version-mismatch drop path is the reason SetResult takes a version, but no test exercised it. Add unit tests for the stale-version drop and the gone-document no-op.
Codex adversarial review found two gaps in the initial cancellation fix:

- Import recursion (buildPartialSchema) swallows ctx.Canceled as a skipped import, so Parse can return err==nil with an incomplete result after a cancel. parseDocument only checked ctx in the err\!=nil branch, so it cached that incomplete result. Move the ctx guard above the result handling so a cancelled request never caches or publishes.

- importDiagnostics threaded ctx but still called parsePathForNavigation, which uses context.Background(), leaking cancellation on the transitive-reimport path. Extract a ctx-aware parsePath (parsePathForNavigation now delegates to it with Background) and use it from importDiagnostics.
Codex follow-up on I2: importDiagnostics still reached the background parser via uniqueImportCandidatePath, which scans and parses every workspace candidate. Thread ctx into uniqueImportCandidatePath — it now bails when the request is cancelled and parses candidates with the request ctx. The diagnostics caller passes the real ctx; interactive code-action callers pass context.Background(), consistent with parsePathForNavigation.
@klaidliadon
klaidliadon force-pushed the ridl-lsp-hardening/parse-pipeline branch from 5c318c4 to 855168c Compare June 20, 2026 07:05
@klaidliadon
klaidliadon merged commit 169223f into master Jun 20, 2026
1 check passed
@klaidliadon
klaidliadon deleted the ridl-lsp-hardening/parse-pipeline branch June 20, 2026 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant